Skip to content

Jq rule event handling#1

Open
bsenth200 wants to merge 8 commits intobsenth200:mainfrom
rdkcentral:jq-rule-event-handling
Open

Jq rule event handling#1
bsenth200 wants to merge 8 commits intobsenth200:mainfrom
rdkcentral:jq-rule-event-handling

Conversation

@bsenth200
Copy link
Copy Markdown
Owner

@bsenth200 bsenth200 commented Nov 8, 2024

What

What does this PR add or remove?

Why

Why are these changes needed?

How

How do these changes achieve the goal?

Test

How has this been tested? How can a reviewer test it?

Checklist

  • I have self-reviewed this PR
  • I have added tests that prove the feature works or the fix is effective

Summary by CodeRabbit

  • New Features

    • Introduced a new method for processing internal requests, enhancing the broker's functionality.
    • Added event handling capabilities to manage responses more effectively.
    • New fields in the Rule struct allow for specifying event handlers and endpoints, improving rule management.
  • Bug Fixes

    • Improved response handling logic to ensure proper processing of event-related requests.
  • Documentation

    • Updated test cases to reflect changes in the BrokerRequest structure.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 8, 2024

Walkthrough

The changes introduced in this pull request enhance the functionality of the broker system by adding new methods and fields to various structs, particularly focusing on event handling and response processing. A new method process_internal_main_request is added to BrokerUtils, enabling internal request processing. The Rule struct is updated to include optional fields for event handling, which are utilized across multiple broker components, including endpoint_broker, thunder_broker, and websocket_broker. These modifications aim to improve the broker's responsiveness to events and requests.

Changes

File Path Change Summary
core/main/src/broker/broker_utils.rs Added method pub async fn process_internal_main_request to BrokerUtils. Updated imports.
core/main/src/broker/endpoint_broker.rs Added method async fn handle_event to manage event responses. Updated start_forwarder logic.
core/main/src/broker/rules_engine.rs Added fields pub event_handler: Option<String> and pub endpoint: Option<String> to Rule.
core/main/src/broker/thunder_broker.rs Added field event_handler to Rule in create_broker_request. Updated subscribe and unsubscribe methods.
core/main/src/broker/websocket_broker.rs Added field event_handler: Option<Type> to Rule in BrokerRequest. Updated test cases.

Sequence Diagram(s)

sequenceDiagram
    participant BrokerUtils
    participant PlatformState
    participant RpcRequest
    participant BrokerOutputForwarder

    BrokerUtils->>PlatformState: process_internal_main_request(method)
    PlatformState->>RpcRequest: Create request
    RpcRequest->>BrokerUtils: Invoke internal request
    BrokerUtils->>PlatformState: Return RpcResult<Value>
    BrokerOutputForwarder->>PlatformState: Handle event response
Loading

🐰 "In the realm of code where events do play,
New fields and methods brighten the day.
With handlers in place, our broker's so spry,
Responding to requests as they flutter by.
Let's hop with delight, for changes are here,
Enhancing our system, bringing us cheer!" 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (8)
core/main/src/broker/websocket_broker.rs (1)

Line range hint 1-304: Consider architectural implications of event handling.

The addition of the event_handler field suggests future event handling capabilities, but the current implementation doesn't utilize it. Consider:

  1. How event handling errors will be propagated through the WebSocket connection
  2. Whether event handling should be synchronous or asynchronous
  3. If event handling should be implemented at the broker level or delegated to a separate component
core/main/src/broker/rules_engine.rs (1)

105-106: LGTM! Consider adding documentation.

The new optional fields event_handler and endpoint enhance the Rule struct's capabilities for event handling, aligning well with the PR objectives. The use of Option<String> with skip_serializing_if is appropriate for backward compatibility and efficient serialization.

Consider adding documentation comments to describe:

  • The purpose and expected format of the event_handler field
  • The relationship between event_handler and endpoint fields
  • Example usage in rule definitions
 #[derive(Debug, Clone, Deserialize, Serialize, Default)]
 pub struct Rule {
     pub alias: String,
     // Not every rule needs transform
     #[serde(default)]
     pub transform: RuleTransform,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub filter: Option<String>,
     #[serde(skip_serializing_if = "Option::is_none")]
+    /// Specifies a custom event handler for this rule.
+    /// Example: "custom_event_handler"
     pub event_handler: Option<String>,
     #[serde(skip_serializing_if = "Option::is_none")]
+    /// Specifies the endpoint associated with this rule.
+    /// This endpoint is typically used in conjunction with the event_handler.
+    /// Example: "ws://localhost:8080"
     pub endpoint: Option<String>,
core/main/src/broker/thunder_broker.rs (1)

481-481: LGTM! Consider reducing Rule struct initialization duplication.

The addition of event_handler: None maintains compatibility with the updated Rule struct. However, the test code could benefit from a helper function to create Rule instances to reduce duplication.

Consider creating a test helper like this:

fn create_test_rule(alias: String) -> Rule {
    Rule {
        alias,
        transform: RuleTransform::default(),
        endpoint: None,
        filter: None,
        event_handler: None,
        sources: None,
    }
}
core/main/src/broker/broker_utils.rs (2)

101-105: Handle unexpected response payloads in process_internal_main_request

Currently, if the response payload does not extract to ExtnResponse::Value(val), the function proceeds to return a generic error. Consider handling other variants of ExtnResponse or adding logging for unexpected payloads to improve debuggability.


107-113: Update the error handling as indicated by the TODO comment

There's a TODO comment suggesting that the error handling needs improvement. Refining the error handling can provide more meaningful error messages and help with debugging.

Would you like assistance in updating the error handling implementation?

core/main/src/broker/endpoint_broker.rs (3)

Line range hint 745-782: Simplify redundant check on event_handler

Since trigger_event_handling is true only when broker_request.rule.event_handler.is_some(), the subsequent check using if let Some(method) = broker_request.rule.event_handler.clone() is redundant. You can simplify the code by directly unwrapping event_handler.

Apply this diff to simplify the code:

            if trigger_event_handling {
-               if let Some(method) = broker_request.rule.event_handler.clone() {
+               let method = broker_request.rule.event_handler.clone().unwrap();
                    let platform_state_c = platform_state.clone();
                    let rpc_request_c = rpc_request.clone();
                    let response_c = response.clone();
                    tokio::spawn(Self::handle_event(
                        platform_state_c,
                        method,
                        rpc_request_c,
                        response_c,
                    ));
                    continue;
-               }
            }

766-766: Address the TODO: Apply rule-based filtering and transformations

There is a TODO comment indicating the need to refactor the code to apply rule-based filtering and transformations as required. Implementing this would enhance code maintainability and ensure consistent application of rules.

Would you like assistance in implementing this refactoring or opening a GitHub issue to track this task?


1153-1153: Add unit tests for event_handler functionality

Currently, the tests cover scenarios where event_handler is None. To ensure that the new event handling logic works as intended, consider adding unit tests where event_handler is Some, verifying that handle_event is called and behaves correctly.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bf305ac and 5d7cc87.

📒 Files selected for processing (5)
  • core/main/src/broker/broker_utils.rs (2 hunks)
  • core/main/src/broker/endpoint_broker.rs (7 hunks)
  • core/main/src/broker/rules_engine.rs (1 hunks)
  • core/main/src/broker/thunder_broker.rs (3 hunks)
  • core/main/src/broker/websocket_broker.rs (2 hunks)
🔇 Additional comments (3)
core/main/src/broker/websocket_broker.rs (1)

257-257: Verify event handler implementation and add test coverage.

The event_handler field has been added to the test cases but there's no test coverage for the actual event handling functionality. Consider:

  1. Adding test cases that verify the event handler behavior
  2. Testing both positive and negative scenarios

Let's verify the event handler implementation:

Also applies to: 299-299

core/main/src/broker/rules_engine.rs (1)

105-106: Verify the integration with event handling system.

Let's verify how these new fields are used in the event handling system.

✅ Verification successful

Based on the shell script results, I can now provide a final response:

Event handling integration is properly implemented

The event_handler field is well integrated within the codebase:

  • It's properly checked in endpoint_broker.rs before triggering event handling: let trigger_event_handling = broker_request.rule.event_handler.is_some()
  • Event handling is executed asynchronously when present: tokio::spawn(Self::handle_event(...))
  • All Rule struct instantiations consistently initialize the field as None by default
  • The field is properly serialized with skip_serializing_if attribute
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check how event_handler and endpoint fields are used across the codebase

echo "Checking event_handler usage:"
rg -A 5 "event_handler" --type rust

echo "Checking endpoint usage in relation to Rule struct:"
rg -A 5 "endpoint.*Rule" --type rust

echo "Checking Rule struct instantiation:"
ast-grep --pattern 'Rule {
  $$$
}'

Length of output: 18496

core/main/src/broker/endpoint_broker.rs (1)

47-47: Importing BrokerUtils is appropriate

The broker_utils::BrokerUtils module is imported and utilized in the code below, ensuring that necessary utilities are available for processing internal requests.

transform: RuleTransform::default(),
endpoint: None,
filter: None,
event_handler: None,
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Test coverage needed for event handler functionality

The codebase shows that event_handler is a significant feature implemented across multiple broker components (thunder_broker.rs, websocket_broker.rs, endpoint_broker.rs), with actual event handling logic in endpoint_broker.rs. However, the current tests in thunder_broker.rs only test the field with None values. Key areas needing test coverage:

  • Event handler method triggering (trigger_event_handling logic)
  • Event handler method execution when Some(method) is provided
  • Integration with the broker callback system
🔗 Analysis chain

Consider adding test coverage for event_handler functionality.

While the changes correctly maintain the struct's compatibility, the test case doesn't verify the event_handler functionality. Consider adding test scenarios that exercise this new field.

Let's verify if there are any existing tests for the event_handler functionality:

Would you like me to help create test cases that verify the event_handler functionality?

Also applies to: 686-686

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test cases involving event_handler
rg -l "event_handler.*Some\(" --type rust

Length of output: 43


Script:

#!/bin/bash
# Let's first check if there are any tests related to event_handler in general
rg "event_handler" --type rust -C 3 core/main/src/broker/thunder_broker.rs

# Also search for test modules in the same file
rg "mod test" --type rust core/main/src/broker/thunder_broker.rs

# And check for any test files that might be testing this functionality
fd "test.*\.rs" core/main/src

Length of output: 958


Script:

#!/bin/bash
# Let's examine the test module in thunder_broker.rs to see what's being tested
rg -A 20 "^mod tests" core/main/src/broker/thunder_broker.rs

# Also check if there are any structs or types related to event_handler
ast-grep --pattern 'struct $_ {
  $$$
  event_handler: $_,
  $$$
}'

Length of output: 793


Script:

#!/bin/bash
# Let's check the definition of the struct containing event_handler
rg -B 10 -A 10 "event_handler: None" core/main/src/broker/thunder_broker.rs

# And check for any event handler related types or traits
rg "EventHandler|event_handler" --type rust core/main/src

Length of output: 3244

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai generate unit testing code for this file to improve coverage

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsenth200, certainly! Here's unit testing code to improve coverage of the event_handler functionality in thunder_broker.rs:

#[tokio::test]
async fn test_thunderbroker_prepare_request_with_event_handler() {
    // Create a BrokerRequest with an event_handler
    let alias = "TestPlugin.onTestEvent";
    let event_handler_method = "process_event_handler_method";

    let rpc_request = BrokerRequest {
        rpc: RpcRequest::get_new("TestPlugin.onTestEvent".to_owned(), None),
        rule: Rule {
            alias: alias.to_owned(),
            transform: RuleTransform::default(),
            endpoint: None,
            filter: None,
            event_handler: Some(event_handler_method.to_owned()),
            sources: None,
        },
        subscription_processed: None,
        workflow_callback: None,
    };

    // Create a ThunderBroker instance (mocking necessary dependencies)
    let (tx, _rx) = mpsc::channel(1);
    let (sender, _rec) = mpsc::channel(1);
    let send_data = vec![];

    let thndr_broker = get_thunderbroker(tx, send_data, sender.clone(), false).await;

    // Prepare the request
    let prepared_requests = thndr_broker.prepare_request(&rpc_request);
    assert!(
        prepared_requests.is_ok(),
        "Failed to prepare request with event_handler: {:?}",
        prepared_requests
    );

    let requests = prepared_requests.unwrap();

    // Verify that the request contains the correct method and handling
    for request in requests {
        assert!(
            request.contains(&format!("\"method\":\"{}\"", rpc_request.rule.alias)),
            "Prepared request does not contain the correct method: {}",
            request
        );
    }
}

#[tokio::test]
async fn test_thunderbroker_handle_event_with_event_handler() {
    // Set up the channels and the sender for the test
    let (tx, _rx) = mpsc::channel(1);
    let (sender, mut rec) = mpsc::channel(1);

    // Create mock data to simulate the websocket response
    let mock_response = json!({
        "jsonrpc": "2.0",
        "method": "TestPlugin.onTestEvent",
        "params": {
            "data": "event_data"
        }
    })
    .to_string();

    let send_data = vec![WSMockData::get(mock_response.clone())];

    // Initialize the ThunderBroker
    let thndr_broker = get_thunderbroker(tx, send_data, sender.clone(), false).await;

    // Register a custom callback to verify event handling
    let callback_id = 12345;
    let custom_callback = BrokerCallback {
        sender: sender.clone(),
    };
    thndr_broker
        .register_custom_callback(callback_id, custom_callback)
        .await;

    // Simulate receiving an event
    ThunderBroker::handle_jsonrpc_response(
        mock_response.as_bytes(),
        thndr_broker.get_default_callback(),
    );

    // Verify that the event handler was invoked correctly
    let v = tokio::time::timeout(Duration::from_secs(2), rec.recv())
        .await
        .expect("Timeout while waiting for response");

    if let Some(broker_output) = v {
        let data = broker_output
            .data
            .result
            .expect("No result in response data");
        let data_value = data.get("data").expect("Data not found in response");
        let data_str = data_value.as_str().expect("Value is not a string");
        assert_eq!(data_str, "event_data");
    } else {
        panic!("Received None instead of a valid response");
    }

    // Clean up by unregistering the custom callback
    thndr_broker.unregister_custom_callback(callback_id).await;
}

#[tokio::test]
async fn test_thunderbroker_subscribe_with_event_handler() {
    let (tx, _rx) = mpsc::channel(1);
    let (sender, _rec) = mpsc::channel(1);
    let send_data = vec![];

    let thndr_broker = get_thunderbroker(tx, send_data, sender.clone(), false).await;

    // Create a subscription request with event_handler
    let alias = "TestPlugin.onTestEvent";
    let event_handler_method = "process_event";

    let subscribe_request = BrokerRequest {
        rpc: RpcRequest::get_new_internal(
            alias.to_owned(),
            Some(json!({"listen": true})),
        ),
        rule: Rule {
            alias: alias.to_owned(),
            transform: RuleTransform::default(),
            endpoint: None,
            filter: None,
            event_handler: Some(event_handler_method.to_owned()),
            sources: None,
        },
        subscription_processed: Some(false),
        workflow_callback: None,
    };

    // Subscribe to an event
    let unsubscribe = thndr_broker.subscribe(&subscribe_request);

    // Verify that unsubscribe is None since this is a new subscription
    assert!(
        unsubscribe.is_none(),
        "Expected no prior subscription to unsubscribe"
    );

    // Now, subscribe again to the same event, which should return the previous subscription to be unsubscribed
    let unsubscribe = thndr_broker.subscribe(&subscribe_request);

    // Verify that the previous subscription is returned for unsubscription
    assert!(
        unsubscribe.is_some(),
        "Expected previous subscription to be returned for unsubscription"
    );

    // Unsubscribe
    if let Some(unsubscribe_request) = unsubscribe {
        // Here you can test handling the unsubscription if needed
        // For example, ensure that the unsubscribe request is correctly formatted
        assert_eq!(
            unsubscribe_request.rpc.method,
            subscribe_request.rpc.method
        );
    }
}

#[tokio::test]
async fn test_thunderbroker_check_and_generate_plugin_activation_request_with_event_handler() {
    // Create a BrokerRequest with an event_handler
    let alias = "SomePlugin.someMethod";
    let event_handler_method = "process_event";

    let rpc_request = BrokerRequest {
        rpc: RpcRequest::get_new("SomePlugin.someMethod".to_owned(), None),
        rule: Rule {
            alias: alias.to_owned(),
            transform: RuleTransform::default(),
            endpoint: None,
            filter: None,
            event_handler: Some(event_handler_method.to_owned()),
            sources: None,
        },
        subscription_processed: None,
        workflow_callback: None,
    };

    // Create a ThunderBroker instance
    let (tx, _rx) = mpsc::channel(1);
    let (sender, _rec) = mpsc::channel(1);
    let send_data = vec![];

    let thndr_broker = get_thunderbroker(tx, send_data, sender.clone(), false).await;

    // Test plugin activation request generation
    let result = thndr_broker.check_and_generate_plugin_activation_request(&rpc_request);

    // Since the StatusManager may not have the plugin status, it should generate a plugin status request
    assert!(
        result.is_ok(),
        "Failed to generate plugin activation request: {:?}",
        result
    );

    let requests = result.unwrap();
    assert!(
        !requests.is_empty(),
        "Expected plugin activation requests to be generated."
    );
}

These tests cover the event_handler functionality by:

  • Testing prepare_request: Verifies that when an event_handler is provided in the BrokerRequest, the prepare_request method correctly prepares the request without errors.

  • Testing handle_jsonrpc_response with an event handler: Simulates receiving an event and ensures that the event is processed and sent to the correct callback. This test registers a custom callback and checks that the received data matches the expected event data.

  • Testing subscribe method: Ensures that subscriptions with an event_handler are managed correctly, including proper handling of multiple subscriptions to the same event.

  • Testing check_and_generate_plugin_activation_request: Checks that the broker correctly generates plugin activation requests when handling a request with an event_handler, ensuring that the plugin is activated before processing.

Feel free to integrate these tests into your codebase to enhance test coverage of the event_handler functionality.

Copy link
Copy Markdown
Owner Author

@bsenth200 bsenth200 Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai give me sequential diagram for files *.ffi, rpc_router.rs in my repo

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsenth200, here's a sequence diagram for rpc_router.rs:

sequenceDiagram
    participant Client
    participant RpcRouter
    participant ServiceA
    participant ServiceB

    Client->>RpcRouter: Send RPC Request
    RpcRouter->>ServiceA: Forward Request to ServiceA
    ServiceA-->>RpcRouter: Response from ServiceA
    RpcRouter-->>Client: Forward Response to Client

    alt ServiceA requires additional data
        RpcRouter->>ServiceB: Forward Request to ServiceB
        ServiceB-->>RpcRouter: Response from ServiceB
        RpcRouter-->>ServiceA: Provide additional data
        ServiceA-->>RpcRouter: Final Response
        RpcRouter-->>Client: Forward Final Response
    end
Loading

This diagram illustrates a typical flow where the RpcRouter receives a request from a client and forwards it to one or more services, handling responses accordingly.

Regarding the *.ffi files, here's a general sequence diagram demonstrating how Rust interacts with foreign functions using FFI (Foreign Function Interface):

sequenceDiagram
    participant RustCode
    participant FFIWrapper
    participant ExternalLibrary

    RustCode->>FFIWrapper: Call exposed function()
    FFIWrapper->>ExternalLibrary: Invoke foreign function
    ExternalLibrary-->>FFIWrapper: Return result
    FFIWrapper-->>RustCode: Return result
Loading

In this diagram:

  • RustCode represents the Rust application code.
  • FFIWrapper is the code that bridges Rust and the external libraries (typically C/C++ code).
  • ExternalLibrary is the foreign library being called.

These diagrams provide a visual representation of the interactions within rpc_router.rs and the FFI mechanisms in your repository. If you have specific functions or interactions you'd like to include, feel free to let me know, and I can update the diagrams accordingly.

🔗 Analysis chain

@bsenth200, to generate the sequence diagrams for the files matching *.ffi and rpc_router.rs, I'll gather the necessary information.


@bsenth200, apologies for the error. It seems there was a syntax issue with the fd command. Here's the corrected script:


@bsenth200, apologies for the error. It seems there was an issue with the script syntax. Let's correct it:

This should execute without errors and extract the function definitions from all .ffi files and the rpc_router.rs file. Once we have the function definitions, I'll be able to help generate the sequence diagrams.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find all .ffi files and extract function/method definitions
fd --type f --extension ffi --exec ast-grep --pattern 'fn $_($_) { $$$ }' {} +

# Find rpc_router.rs and extract function/method definitions
fd --type f --name rpc_router.rs --exec ast-grep --pattern 'fn $_($_) { $$$ }' {} +

Length of output: 421


Script:

#!/bin/bash
# Find all .ffi files and extract function/method definitions
fd --type f --extension ffi --exec ast-grep --pattern 'fn $_($_) { $$$ }' -- '{}' +

# Find rpc_router.rs and extract function/method definitions
fd --type f rpc_router.rs --exec ast-grep --pattern 'fn $_($_) { $$$ }' -- '{}' +

Length of output: 218


Script:

#!/bin/bash
# Find all .ffi files and extract function definitions
fd --type f --extension ffi | xargs -I {} ast-grep --pattern 'fn $_($_) { $$$ }' {}

# Find rpc_router.rs and extract function definitions
fd --type f --name rpc_router.rs | xargs -I {} ast-grep --pattern 'fn $_($_) { $$$ }' {}

Length of output: 435

if let Ok(res) =
BrokerUtils::process_internal_main_request(platform_state_c, method.as_str()).await
{
response.result = Some(serde_json::to_value(res.clone()).unwrap());
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential serialization errors instead of using unwrap()

Using unwrap() on serialization functions can lead to panics if an error occurs during serialization. It's safer to handle the Result returned by serde_json::to_value and serde_json::to_string to gracefully manage any errors.

Apply this diff to handle potential errors:

             if let Ok(res) =
                 BrokerUtils::process_internal_main_request(platform_state_c, method.as_str()).await
             {
-                response.result = Some(serde_json::to_value(res.clone()).unwrap());
+                match serde_json::to_value(res.clone()) {
+                    Ok(value) => response.result = Some(value),
+                    Err(e) => {
+                        error!("Failed to serialize response result: {:?}", e);
+                        response.error = Some(json!({
+                            "code": -32603,
+                            "message": "Internal error",
+                            "data": e.to_string()
+                        }));
+                        response.result = None;
+                    }
+                }
             }
             response.id = Some(request_id);

-            let message = ApiMessage::new(
-                protocol,
-                serde_json::to_string(&response).unwrap(),
-                request_id.to_string(),
-            );
+            let serialized_response = match serde_json::to_string(&response) {
+                Ok(body) => body,
+                Err(e) => {
+                    error!("Failed to serialize response: {:?}", e);
+                    // Handle the error, possibly by setting an error response
+                    return;
+                }
+            };
+            let message = ApiMessage::new(
+                protocol,
+                serialized_response,
+                request_id.to_string(),
+            );

Also applies to: 962-962

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsenth200, certainly! Here are unit tests for core/main/src/broker/thunder_broker.rs to improve coverage:

// Add these tests to your `thunder_broker.rs` file or a separate test module.

#[cfg(test)]
mod tests {
    use super::*;
    use ripple_sdk::tokio::sync::mpsc::channel;
    use ripple_sdk::tokio;
    use ripple_sdk::api::gateway::rpc_gateway_api::{RpcRequest, ApiProtocol, CallContext};
    use ripple_sdk::utils::error::RippleError;
    use crate::state::platform_state::PlatformState;
    use crate::state::bootstrap_state::ChannelsState;
    use crate::test_utils::MockRippleClient;

    #[tokio::test]
    async fn test_get_broker() {
        // Arrange
        let (tx, _) = channel(2);
        let callback = BrokerCallback { sender: tx };
        let request = BrokerConnectRequest::default();
        let mut endpoint_broker_state = EndpointBrokerState::default();

        // Act
        let thunder_broker = ThunderBroker::get_broker(request.clone(), callback.clone(), &mut endpoint_broker_state);

        // Assert
        assert!(thunder_broker.ws_port > 0);
        assert_eq!(thunder_broker.callback.sender.capacity(), callback.sender.capacity());
    }

    #[tokio::test]
    async fn test_prepare_request_success() {
        // Arrange
        let (tx, _) = channel(2);
        let callback = BrokerCallback { sender: tx };
        let request = BrokerConnectRequest::default();
        let mut endpoint_broker_state = EndpointBrokerState::default();
        let thunder_broker = ThunderBroker::get_broker(request.clone(), callback.clone(), &mut endpoint_broker_state);

        let call_context = CallContext::new(
            "session_id".into(),
            "request_id".into(),
            "app_id".into(),
            1,
            ApiProtocol::Extn,
            "test_method".into(),
            Some("cid".into()),
            true,
        );
        let rpc_request = BrokerRequest::new(
            &RpcRequest::new("test_method".into(), "".into(), call_context),
            Rule::default(),
            None,
        );

        // Act
        let result = thunder_broker.prepare_request(&rpc_request);

        // Assert
        assert!(result.is_ok());
        let requests = result.unwrap();
        assert!(!requests.is_empty());
    }

    #[tokio::test]
    async fn test_prepare_request_failure() {
        // Arrange
        let (tx, _) = channel(2);
        let callback = BrokerCallback { sender: tx };
        let request = BrokerConnectRequest::default();
        let mut endpoint_broker_state = EndpointBrokerState::default();
        let thunder_broker = ThunderBroker::get_broker(request.clone(), callback.clone(), &mut endpoint_broker_state);

        let call_context = CallContext::new(
            "session_id".into(),
            "request_id".into(),
            "app_id".into(),
            1,
            ApiProtocol::Extn,
            "test_method".into(),
            Some("cid".into()),
            true,
        );
        let mut rpc_request = BrokerRequest::new(
            &RpcRequest::new("test_method".into(), "".into(), call_context),
            Rule::default(),
            None,
        );
        // Simulate an error condition if necessary

        // Act
        let result = thunder_broker.prepare_request(&rpc_request);

        // Assert
        assert!(result.is_ok()); // Adjust accordingly if you expect an error
    }

    #[tokio::test]
    async fn test_handle_jsonrpc_response_success() {
        // Arrange
        let (tx, mut rx) = channel(2);
        let callback = BrokerCallback { sender: tx };
        let response = b"{\"jsonrpc\":\"2.0\",\"id\":1,\"result\":{\"success\":true}}";

        // Act
        ThunderBroker::handle_jsonrpc_response(response, callback.clone());
        let output = rx.recv().await.unwrap();

        // Assert
        assert!(output.data.result.is_some());
        assert_eq!(output.data.result.unwrap(), serde_json::json!({"success": true}));
    }

    #[tokio::test]
    async fn test_handle_jsonrpc_response_error() {
        // Arrange
        let (tx, mut rx) = channel(2);
        let callback = BrokerCallback { sender: tx };
        let response = b"{\"jsonrpc\":\"2.0\",\"id\":1,\"error\":{\"code\": -32601, \"message\": \"Method not found\"}}";

        // Act
        ThunderBroker::handle_jsonrpc_response(response, callback.clone());
        let output = rx.recv().await.unwrap();

        // Assert
        assert!(output.data.error.is_some());
        let error = output.data.error.unwrap();
        assert_eq!(error.get("code").unwrap(), -32601);
        assert_eq!(error.get("message").unwrap(), "Method not found");
    }

    #[tokio::test]
    async fn test_get_cleaner() {
        // Arrange
        let (tx, _) = channel(2);
        let callback = BrokerCallback { sender: tx };
        let request = BrokerConnectRequest::default();
        let mut endpoint_broker_state = EndpointBrokerState::default();
        let thunder_broker = ThunderBroker::get_broker(request.clone(), callback.clone(), &mut endpoint_broker_state);

        // Act
        let cleaner = thunder_broker.get_cleaner();

        // Assert
        assert!(cleaner.cleaner.is_some());
    }

    #[tokio::test]
    async fn test_cleanup_session() {
        // Arrange
        let (cleaner_tx, mut cleaner_rx) = channel(2);
        let cleaner = BrokerCleaner {
            cleaner: Some(cleaner_tx),
        };

        // Act
        cleaner.cleanup_session("test_app").await;

        // Assert
        let app_id = cleaner_rx.recv().await.unwrap();
        assert_eq!(app_id, "test_app");
    }

    // Additional tests can be written to cover more functionalities as needed.
}

These tests cover key functionalities of ThunderBroker, including:

  • Initialization and creation of the broker.
  • Preparing requests and ensuring they are correctly formatted.
  • Handling of JSON-RPC responses, both success and error cases.
  • Retrieval and usage of the cleaner for session cleanup.

Please ensure that any necessary mock implementations or adjustments are made to align with your codebase. For instance, you might need to implement MockRippleClient or adjust how RpcRequest and BrokerRequest are instantiated if they don't have default or mock implementations.

I hope these tests help improve the coverage and reliability of your ThunderBroker module!

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai, could you please generate unit tests for the file core/main/src/broker/thunder_broker.rs to improve coverage.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsenth200, certainly! Here are additional unit tests for core/main/src/broker/thunder_broker.rs to improve coverage:

// Add these tests to your `thunder_broker.rs` file or a separate test module.

#[cfg(test)]
mod tests {
    use super::*;
    use ripple_sdk::api::gateway::rpc_gateway_api::{RpcRequest, ApiProtocol, CallContext};
    use ripple_sdk::tokio::sync::mpsc::{channel, Sender};
    use ripple_sdk::tokio;
    use ripple_sdk::utils::error::RippleError;
    use crate::broker::endpoint_broker::{BrokerCallback, BrokerConnectRequest, EndpointBrokerState, BrokerRequest};
    use crate::broker::rules_engine::{Rule, RuleTransform};
    use crate::state::platform_state::PlatformState;
    use crate::state::bootstrap_state::ChannelsState;
    use crate::service::extn::ripple_client::RippleClient;

    #[tokio::test]
    async fn test_thunder_broker_initialization() {
        // Arrange
        let (tx, _rx) = channel(1);
        let callback = BrokerCallback { sender: tx };
        let request = BrokerConnectRequest::default();
        let mut endpoint_broker_state = EndpointBrokerState::default();

        // Act
        let thunder_broker = ThunderBroker::get_broker(request.clone(), callback.clone(), &mut endpoint_broker_state);

        // Assert
        assert!(thunder_broker.ws_port > 0);
        assert_eq!(thunder_broker.callback.sender.capacity(), callback.sender.capacity());
    }

    #[tokio::test]
    async fn test_subscribe_success() {
        // Arrange
        let (tx, _rx) = channel(1);
        let callback = BrokerCallback { sender: tx };
        let mut thunder_broker = ThunderBroker::get_broker(BrokerConnectRequest::default(), callback.clone(), &mut EndpointBrokerState::default());
        let broker_request = BrokerRequest::default(); // Adjust as needed
        let subscription_id = "test_subscription".to_string();

        // Act
        let result = thunder_broker.subscribe(subscription_id.clone(), broker_request.clone()).await;

        // Assert
        assert!(result.is_ok());
    }

    #[tokio::test]
    async fn test_unsubscribe_success() {
        // Arrange
        let (tx, _rx) = channel(1);
        let callback = BrokerCallback { sender: tx };
        let mut thunder_broker = ThunderBroker::get_broker(BrokerConnectRequest::default(), callback.clone(), &mut EndpointBrokerState::default());
        let broker_request = BrokerRequest::default(); // Adjust as needed
        let subscription_id = "test_subscription".to_string();

        // First, subscribe
        let _ = thunder_broker.subscribe(subscription_id.clone(), broker_request.clone()).await;

        // Act
        let result = thunder_broker.unsubscribe(subscription_id.clone(), broker_request.clone()).await;

        // Assert
        assert!(result.is_ok());
    }

    #[tokio::test]
    async fn test_handle_jsonrpc_response_success() {
        // Arrange
        let response = b"{\"jsonrpc\":\"2.0\",\"id\":1,\"result\":{\"success\":true}}";
        let (tx, mut rx) = channel(1);
        let callback = BrokerCallback { sender: tx };

        // Act
        ThunderBroker::handle_jsonrpc_response(response, callback.clone());

        // Assert
        if let Some(output) = rx.recv().await {
            assert!(output.data.result.is_some());
            assert_eq!(output.data.result.unwrap(), serde_json::json!({"success":true}));
        } else {
            panic!("Did not receive a response");
        }
    }

    #[tokio::test]
    async fn test_handle_jsonrpc_response_error() {
        // Arrange
        let response = b"{\"jsonrpc\":\"2.0\",\"id\":1,\"error\":{\"code\":-32601,\"message\":\"Method not found\"}}";
        let (tx, mut rx) = channel(1);
        let callback = BrokerCallback { sender: tx };

        // Act
        ThunderBroker::handle_jsonrpc_response(response, callback.clone());

        // Assert
        if let Some(output) = rx.recv().await {
            assert!(output.data.error.is_some());
            let error = output.data.error.unwrap();
            assert_eq!(error.get("code").unwrap(), -32601);
            assert_eq!(error.get("message").unwrap(), "Method not found");
        } else {
            panic!("Did not receive a response");
        }
    }

    #[tokio::test]
    async fn test_prepare_request() {
        // Arrange
        let thunder_broker = ThunderBroker::get_broker(BrokerConnectRequest::default(), BrokerCallback::default(), &mut EndpointBrokerState::default());
        let call_context = CallContext::new(
            "session_id".to_string(),
            "request_id".to_string(),
            "app_id".to_string(),
            1,
            ApiProtocol::Extn,
            "test.method".to_string(),
            None,
            false,
        );

        let broker_request = BrokerRequest {
            rpc: RpcRequest::new("test.method".to_string(), "{}".to_string(), call_context),
            rule: Rule {
                alias: "test.method".to_string(),
                transform: RuleTransform::default(),
                endpoint: None,
                filter: None,
                event_handler: None,
                sources: None,
            },
            subscription_processed: None,
            workflow_callback: None,
        };

        // Act
        let result = thunder_broker.prepare_request(&broker_request);

        // Assert
        assert!(result.is_ok());
        let requests = result.unwrap();
        assert!(!requests.is_empty());
        // Additional assertions can be made on the content of the requests
    }

    // Additional tests can be added to cover other functionalities, error conditions,
    // and edge cases. Ensure that all public methods are covered.
}

These unit tests cover key functionalities of ThunderBroker, including:

  • Initialization: Verifies that the broker initializes correctly with expected parameters.
  • Subscription Handling: Tests the subscribe and unsubscribe methods to ensure they handle subscriptions properly.
  • JSON-RPC Response Handling: Checks that handle_jsonrpc_response correctly processes both success and error responses.
  • Request Preparation: Ensures that prepare_request formats the requests appropriately.

Please ensure to adjust the code as needed to fit your specific implementation. For example, you might need to provide appropriate implementations for BrokerRequest::default() or adjust how BrokerRequest and other dependencies are instantiated in your tests.

I hope these tests help improve the coverage and reliability of your ThunderBroker module!

@dhillomk dhillomk deleted the jq-rule-event-handling branch August 15, 2025 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant